-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
web-wallet: Overhaul landing page #1536
Conversation
de40a2c
to
a840ebb
Compare
cc0aee4
to
f4b1964
Compare
a5523ee
to
6c9e00f
Compare
Wondering if it won't be better to just have the landing page under the |
63aedec
to
0cab69c
Compare
0cab69c
to
2fcb6ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some notes as there are some problems.
-
these changes should have been made in three separate PRs
-
pressing enter in the unlock textbox doesn't always unlock the wallet:
in some cases it seems that it just fires a blur event on the textbox -
if I set up a password, enter my wallet, go to settings and reset the wallet
- login info is not deleted: new bug? regression we had from before?
- the system sees existing login info, unlock screen asks for password,
but by entering the password I'm redirected to the terms & privacy policy in the restore flow.
By accepting the terms I see my password in clear as the first word of the mnemonic (see screenshot)
with no possibility to advance further.
-
if I enter a new mnemonic and with an existing wallet, and I accept to proceed by restoring another wallet,
I'm brought to the "enter mnemonic phrase" step of the restore flow, but the mnemonic is now in clear
while just seconds ago it was hidden.
- should we skip this step?
- should we show it, but obfuscated by default with an option to show it? Should we consider always obfuscating it by default (or maybe only after a paste) as others are doing? (this is for a new issue anyway) -
didn't do a full review of the code yet, as there are some problems and we're waiting for @ZER0's input,
but I thinkFieldButtonGroup
is quite a confusing name for that component.
Given that for now the component is specific to the application, I would go for something along the lines of
"UnlockTexbox", or anyway something mentioning that is the control for unlocking. -
at this point it's probably better to move the
TermsOfService
in the app components? -
it's fine having a
redirect(301, "/")
in the setup route right now, but maybe we should consider
trapping all404
s in client routing instead and make the same redirect to the home? (in a future issue)
What do you think?
We could have done, but I also see the work around the login page as a whole. For the rest of the allowing a sync from a custom block height epic, I will work with Matteo to organize the features in a way that makes sense to deliver them. From yesterday's discussion, seems like we could do the individual parts separately – I would have personally used an feature branch to capture the whole initiative (with smaller branches containing the individual tasks), so to have the whole thing delivered at once.
Will investigate. I think I saw some issues with Chrome, which were not present in Safari.
I haven't changed anything in the settings page, but I also did not manually test this flow. This is a good catch, I will check what is going on. Also, seeing your password as the first word of the mnemonic, obviously, shouldn't happen, will fix this.
As you said, let's open a new issue and involve the others in the discussion.
I have pinged Matteo on Discord to have a look. I assume it will be after the consensus update has been rolled out.
It is not specific to the application, the same component will be used in the Explorer too, thus the generic component name. Another option could be
I'm fine with that.
I was just thinking from SEO perspective – if a search engine has indexed a route that we have gotten rid of, I think we should respond with the appropriate code – at least initially when we release the change. |
I think so, yes. (cc @kieranhall @deuch13 ) |
I agree with this, as I said in my earlier comment I would go even further and have both login and restore flow under the same route. |
I thought it was under
Yes, let's leave it like you did as right now we don't have the option to modify things server side, and this way the route exists even for the server. |
Closing this for the time being, as this has been put on hold without clear idea on when the change will be needed. |
Resolves #1534